PR Review — Migrate integration and provider HTTP types (PR 13)

1 blocking finding, 5 non-blocking suggestions.

Blocking

🔧 Missing Content-Type forwarding for Didomi POST/PUT — see inline comment.

Non-blocking (inline)

• 🤔 Prebid still imports from fastly::http — last remaining use fastly in the integration layer
• ♻️ Duplicated body collection helpers (collect_response_body / collect_body_bytes)
• 🌱 Unbounded response body collection (matches pre-migration, but could use a size cap)
• ⛏ Dead "X-" uppercase branch in custom header copy (http crate lowercases names)
• ⛏ Duplicated backend_name_for_url helper across 4 integrations

ignore this

Summary

Solid migration of the integration and auction provider layers from Fastly-specific HTTP types to platform-agnostic http crate types. No blocking issues found. CI is passing.

Non-blocking

♻️ refactor

• Duplicated body_as_reader helper: proxy.rs:24 and publisher.rs:23 define identical body_as_reader functions. Consider extracting into a shared location (e.g., an into_reader() method on EdgeBody, or a helper in http_util.rs).

📝 note

• PR description file table incomplete: proxy.rs and publisher.rs have changes in this diff (the body_as_reader extraction and test type alias cleanups) but are not listed in the PR body's change table.

📌 out of scope

• Orchestrator tests disabled pending mock PlatformHttpClient: Several provider integration tests and timeout enforcement paths in orchestrator.rs:751-765 are disabled pending a mock for PlatformHttpClient::select(). This is acknowledged in the PR, but the scope of untested paths has grown — a follow-up to add thin mock support for select() would significantly improve coverage of the deadline enforcement logic.

Summary

The refactor is clean and the ensure_integration_backend / collect_body helpers extracted in the fix commits are a genuine improvement. The main risks are latent panics and missing POST size bounds that were introduced in the base migration commit. None are new in the fix commits, but this PR is the natural owner of the HTTP-type boundary, so they should land before merge.

Blocking

🔧 wrench

• EdgeBody::into_bytes() panics on streaming bodies at auction/endpoints.rs:42 — the inbound auction POST body is user-controlled. Body::into_bytes() panics on Body::Stream(_) (unit test into_bytes_panics_for_stream asserts this). Today Fastly's adapter happens to return Body::Once, so nothing panics — but that invariant is undocumented and the /auction handler is the first publicly-reachable crash if it ever changes. Fix: switch to Body::into_bytes_bounded(max).await with a documented cap (e.g. 256KB).
• EdgeBody::into_bytes() panics across every migrated provider/proxy — same root cause, see inline comments on prebid.rs:1157, permutive.rs:151, lockr.rs:149, datadome.rs:301, aps.rs:554, adserver_mock.rs:373. Make parse_response async and read bodies with into_bytes_bounded.
• Unbounded inbound POST body forwarding on the datadome / didomi / permutive / lockr proxies — only GTM enforces a cap. Promote the GTM size-bounded reader into integrations/mod.rs as collect_body_bounded and apply it uniformly. Inline comments on datadome.rs:367, didomi.rs:245, permutive.rs:211, lockr.rs:203.
• Lockr .expect() on user-controlled Origin header — reachable worker panic at lockr.rs:266.
• GTM stream-read errors mis-classified as PayloadTooLarge — returns 413 for transport errors at google_tag_manager.rs:313. Add a StreamRead variant and map to 502.

Non-blocking

🤔 thinking

• collect_body has no size cap (integrations/mod.rs:101) — silently drains whole bodies to RAM. Same concern applies to the GTM response-rewrite path at google_tag_manager.rs:486.
• Orchestrator select_result.ready error path drops provider identity in run_providers_parallel — when ready is Err, the provider identity is lost so no AuctionResponse::error(...) is pushed and the failing backend vanishes silently from the result set. Track a (backend_name, provider_index) pair alongside fastly_pending so failures are attributable.
• Deadline enforcement relies on every provider honoring backend_name(effective_timeout) — Phase 1 computes remaining_ms.min(provider.timeout_ms()) correctly, but nothing guarantees each provider actually threads that timeout through to the backend config. Not a new regression, but the async migration makes it more load-bearing. Add a unit test that asserts the select loop returns before deadline + epsilon.

♻️ refactor

• aps.rs / adserver_mock.rs still build BackendConfig inline — extend the new ensure_integration_backend helper to cover providers too.

🌱 seedling

• Make parse_response async in the provider trait (auction/provider.rs:48) — needed anyway once the into_bytes_bounded fix lands; zero cost under #[async_trait(?Send)].

📝 note

• Testlight stale Content-Length test is narrow (testlight.rs:425) — good unit-level coverage, but the two handle() rebuild call sites would benefit from an end-to-end assertion.

👍 praise

• Clean helper extraction in integrations/mod.rs — real dedup across six integrations.
• Dropping the dead || name_str.starts_with("X-") branch in lockr.rs / permutive.rs — HeaderName::as_str() is always lowercase, so it was unreachable.
• Adding CONTENT_TYPE to didomi's forwarded headers — POST bodies can now be interpreted by upstream.

CI Status

• browser integration tests: PASS
• integration tests: PASS
• prepare integration artifacts: PASS

Summary

Re-review of the EdgeZero PR13 integration/provider HTTP type migration after the latest "Address review findings" commit. The async parse_response migration and the new collect_body / collect_body_bounded helpers landed cleanly and resolved the prior into_bytes()-on-Stream panic class across all providers. Two blockers remain plus one CI/process question that affects whether the existing test plan can be trusted.

Blocking

🔧 wrench

• cargo fmt --all -- --check fails locally in 7+ files. The PR description's [x] cargo fmt checkbox is incorrect — see the CI question below for why this wasn't caught. Affected files:

  • crates/trusted-server-core/src/auction/endpoints.rs:39-47
  • crates/trusted-server-core/src/auction/orchestrator.rs:12-14, 402-405
  • crates/trusted-server-core/src/integrations/google_tag_manager.rs:16-25, 39-45
  • crates/trusted-server-core/src/integrations/lockr.rs:282-285
  • crates/trusted-server-core/src/integrations/permutive.rs:208-219, 278-281
  • crates/trusted-server-core/src/integrations/prebid.rs:5-13, 1154-1162
  • crates/trusted-server-core/src/integrations/testlight.rs:11-16

  Fix: run cargo fmt --all.

• Testlight POST body is unbounded (inline at integrations/testlight.rs:181).

❓ question

• CI gates aren't enforced for this PR. .github/workflows/format.yml and .github/workflows/test.yml only fire on pull_request: branches: [main]. PR 626's base is feature/edgezero-pr12-handler-layer-types, so on commit 94ec9477 only "Integration Tests" ran — fmt, clippy, and cargo test --workspace did not. Combined with the fmt failure above, every stacked EdgeZero PR ships with broken formatting and an unverified test plan. Was this intentional, or should the workflow triggers be widened (or the gates duplicated under the integration-tests umbrella)? Until that's resolved, the PR description's CI checkboxes can't be relied on as evidence.

  Verified locally on this commit: cargo test --workspace passes (821 tests), cargo clippy --workspace --all-targets --all-features -- -D warnings passes, cargo build --release --target wasm32-wasip1 passes, cargo fmt --all -- --check fails.

Non-blocking

🤔 thinking

• Unbounded response-body reads via collect_body (inline at integrations/mod.rs:93 for stream-read error fidelity; cross-cutting concern listed here). The bounded variant exists for inbound bodies, but every upstream response site still drains an unbounded body to a Vec<u8>:

  • integrations/prebid.rs:1157 — Prebid Server response (highest risk: third-party hop, large payloads)
  • integrations/aps.rs:555 — APS response
  • integrations/adserver_mock.rs:375 — Mediator response
  • integrations/lockr.rs:150 — Lockr SDK fetch
  • integrations/permutive.rs:152 — Permutive SDK fetch
  • integrations/datadome.rs:302 — DataDome SDK fetch
  • integrations/google_tag_manager.rs:491 — GTM script

  Provider responses are the highest risk — a misbehaving Prebid Server / mediator can ship a multi-GB body and OOM the worker. Recommend routing all of these through collect_body_bounded with a per-callsite cap, or removing the unbounded variant in favor of a single bounded helper.

♻️ refactor

• Auction providers still construct BackendConfig inline while integration proxies route through ensure_integration_backend. Reraising the prior-review concern with current locations:

  • integrations/aps.rs:517-528
  • integrations/adserver_mock.rs:338-348
  • integrations/prebid.rs:1131-1135

  The two paths differ on timeout (providers need a configurable per-request timeout; the proxy helper hardcodes 15s). Consider widening ensure_integration_backend to accept a timeout and reusing it from the providers, or at least documenting the divergence.

CI Status

• fmt: FAIL locally (CI did not run for this commit — see ❓ question above)
• clippy: PASS locally (CI did not run)
• rust tests: PASS locally — 821 tests (CI did not run)
• wasm32-wasip1 build: PASS locally
• Integration Tests workflow: PASS (only workflow that ran on 94ec9477)

Summary

Re-review of PR13 after the latest "Address PR review" commit (4b27087e) and the PR12 merge (a2036eb5). The async parse_response migration, the dedicated AUCTION_MAX_BODY_BYTES, the README updates, and the testlight body cap all landed cleanly. One blocker remains plus several non-blocking re-raises from prior reviews that haven't been addressed.

Local verification on this commit:

• cargo test --workspace — PASS (952 tests, 0 failures)
• cargo clippy --workspace --all-targets --all-features -- -D warnings — PASS
• cargo build --release --target wasm32-wasip1 -p trusted-server-adapter-fastly — PASS
• cargo fmt --all -- --check — FAIL (4 PR-introduced sites + 11 inherited from PR12 base)
• GitHub CI (integration tests workflow) — PASS

GitHub's format.yml / test.yml still only fire on PRs targeting main, so the fmt/clippy/test gates don't run for this PR's stack. The PR description's [x] cargo fmt / [x] cargo clippy / [x] cargo test checkboxes therefore can't be relied on as evidence — see the fmt blocker below.

Blocking

🔧 wrench

• fmt failures still present in PR-modified files. cargo fmt --all -- --check reports diffs at:

  • crates/trusted-server-adapter-fastly/src/main.rs:209 and :236
  • crates/trusted-server-core/src/integrations/prebid.rs:1057 (the line at ~1066 is too long inside the if let block)
  • crates/trusted-server-core/src/integrations/registry.rs:673

  These four sites did not exist on the PR12 base — PR13 introduced them. (11 additional fmt diffs are inherited from the PR12 base and are not this PR's responsibility, but the project-wide cargo fmt --all -- --check does fail.) Fix: cargo fmt --all.

Non-blocking

🤔 thinking

• Provider/integration response bodies still unbounded (re-raise; inline at integrations/mod.rs:84). 7 callsites still drain upstream responses with collect_body instead of a bounded helper. Provider responses are highest risk (prebid.rs:1160, aps.rs:555, adserver_mock.rs:375).
• Stream-read errors classified as Integration (re-raise; inline at integrations/mod.rs:93 and the bounded variant at :137). Conflates transport failure with integration semantic error; both surface as 5xx-server with the same shape. GTM has a bespoke 502 path that the shared helper doesn't.
• Orchestrator backend-name fallback can silently lose bids (inline at auction/orchestrator.rs:337). When pending.backend_name() is None and the platform sets a different PlatformResponse::backend_name, the lookup at line 392 fails and the bid is dropped with a "Received response from unknown backend" warning. Consider a log::warn! when the fallback fires.
• collect_body_bounded checks size after reading the chunk (inline at integrations/mod.rs:143). The effective bound is "≤ max + one_chunk", not strict ≤ max. Acceptable in practice (chunk sizes are small) but worth a comment on the helper.

♻️ refactor

• Auction providers still construct BackendConfig inline (re-raise; inline at prebid.rs:1134, also aps.rs:517-528, adserver_mock.rs:338-348). Diverges from ensure_integration_backend because providers need a per-request timeout and the helper hardcodes 15s.
• AuctionContext.client_info is redundant with services.client_info() (re-raise; inline at auction/types.rs:107).
• run_auction takes services as a separate arg even though context.services exists (inline at auction/orchestrator.rs:64). They must stay in sync; an inconsistent caller breaks the auction silently.
• Inconsistent services.client_info access (inline at didomi.rs:259). Two sites use the pub(crate) field (didomi.rs:259, auction/formats.rs:145); the rest use the method services.client_info(). Standardize on the method.

📝 note

• Lockr/Permutive/Didomi handlers contain unreachable PUT/PATCH branches (inline at lockr.rs:202). Routes register only GET/POST.
• StubHttpClient::send_async does not capture request headers (inline at platform/test_support.rs:297). Limits header-shape assertions for prebid/aps/adserver_mock tests.

🏕 camp site

• testlight::Default impls can be derived (inline at testlight.rs:271).

👍 praise

• compat:: shim eliminated from integration dispatch — IntegrationProxy::handle is now http::Request<EdgeBody> end-to-end (main.rs:195, registry.rs:266). Removes a non-trivial conversion overhead per request and a class of header-mapping bugs.
• Async parse_response migration with PlatformResponse resolves the prior into_bytes()-on-Body::Stream panic class for every provider (auction/provider.rs:38). The trait docs explicitly note why it's now async — exactly the right context for future implementors.
• GTM 413 vs 502 distinction (google_tag_manager.rs:511-535). Clean separation between client-error and transport-error; this is the pattern the shared collect_body* helpers should follow.
• EC-ID header-injection guard (registry.rs:673) plus regression test at registry.rs:1406. Defensive HeaderValue::from_str blocks \r/\n/\0 injection on a user-influenced ID before it lands in either the request or the response.

CI Status

• fmt: FAIL locally (CI did not run for this commit because workflow triggers don't match the PR's base branch — see header above)
• clippy: PASS locally (CI did not run)
• rust tests: PASS locally (CI did not run; 952 tests)
• wasm32-wasip1 build: PASS locally
• Integration Tests workflow: PASS

Inline annotations for the prior REQUEST_CHANGES review

Adding the per-line comments now that I've filtered them against the PR diff (the previous submission was body-only because GitHub rejected the batch when 3 of 17 comment anchors fell outside the changed hunks). These reference the same findings as #626 (review) — no new findings, just inline anchoring.

The three findings that couldn't be anchored to a diff line are tagged below for reference:

• ♻️ refactor — run_auction takes services separately even though context.services exists. Anchored as a top-level note rather than inline because auction/orchestrator.rs:64 (the run_auction signature) is not in the diff, but the dual-pass pattern is visible at the response-handling sites (e.g. orchestrator.rs:337).
• 📝 note — StubHttpClient::send_async does not capture request headers. crates/trusted-server-core/src/platform/test_support.rs is not in this PR's diff scope at all; the asymmetry between send (lines 266–280) and send_async (lines 297–320) limits header-shape assertions for prebid/aps/adserver_mock tests. Worth a follow-up issue.
• 🏕 camp site — testlight::Default impls can be derived. testlight.rs:271-285 is outside the changed hunks; deriving #[derive(Default)] would replace both manual impls.

Inline annotations for the prior REQUEST_CHANGES review

Adding the per-line comments now that I've filtered them against the PR diff (the previous submission was body-only because GitHub rejected the batch when 3 of 17 comment anchors fell outside the changed hunks). These reference the same findings as #626 (review) — no new findings, just inline anchoring.

The three findings that couldn't be anchored to a diff line are tagged below for reference:

• ♻️ refactor — run_auction takes services separately even though context.services exists. Anchored as a top-level note rather than inline because auction/orchestrator.rs:64 (the run_auction signature) is not in the diff, but the dual-pass pattern is visible at the response-handling sites (e.g. orchestrator.rs:337).
• 📝 note — StubHttpClient::send_async does not capture request headers. crates/trusted-server-core/src/platform/test_support.rs is not in this PR's diff scope at all; the asymmetry between send (lines 266–280) and send_async (lines 297–320) limits header-shape assertions for prebid/aps/adserver_mock tests. Worth a follow-up issue.
• 🏕 camp site — testlight::Default impls can be derived. testlight.rs:271-285 is outside the changed hunks; deriving #[derive(Default)] would replace both manual impls.

Summary

Re-review of the EdgeZero PR13 integration/provider HTTP type migration after the latest commit (96949807). The async parse_response, collect_response_bounded, and dedicated AUCTION_MAX_BODY_BYTES constant landed cleanly and resolved several prior findings. Three blocking issues remain, all rooted in the same anti-pattern: change_context over collect_body_bounded discards the RequestTooLarge error variant, so oversized inbound bodies surface as 502 BAD_GATEWAY instead of 413 PAYLOAD_TOO_LARGE at every site except testlight. A test added in this PR (auction_rejects_oversized_body) already proves the bug.

CI status (verified locally on 96949807)

• fmt: PASS
• clippy: FAIL — 3 doc_markdown errors at integrations/mod.rs:172, 175
• rust tests: FAIL — auction_rejects_oversized_body panics (asserts 413, gets 502)
• wasm32-wasip1 release build: PASS
• JS tests: PASS — 282 tests
• Integration Tests workflow: PASS (only workflow that ran on this commit)

The PR description's [x] cargo clippy and [x] cargo test --workspace checkboxes are not corroborated by CI on this branch — .github/workflows/{format,test}.yml only fire on pull_request: branches: [main], so on commit 96949807 only the integration-tests workflow ran. Both gates fail when run locally.

Blocking

🔧 wrench

• cargo clippy --workspace --all-targets --all-features -- -D warnings fails at integrations/mod.rs:172, 175 (3 doc-markdown errors).
• auction_rejects_oversized_body fails because auction/endpoints.rs:46 change_contexts RequestTooLarge (→ 413) into Auction (→ 502).
• Same bug at every other collect_body_bounded caller except testlight: datadome.rs:362-364, didomi.rs:244-246, permutive.rs:213-217, lockr.rs:205-207. All produce 502 instead of 413 for oversized bodies, defeating the bounded-read security goal. testlight.rs:182-184 is the only correct caller — its ?-only pattern is the model.

Non-blocking

🤔 thinking

• Unbounded upstream-response read in testlight (testlight.rs:214). Every other RTB integration migrated to collect_response_bounded; testlight is the last unbounded site.
• Predicted-backend-name fallback at log::debug (orchestrator.rs:339-344). Failure mode is silent bid loss; recommend log::warn! so operators can see the fallback firing.

♻️ refactor

• Three providers still inline BackendConfig::from_url_with_first_byte_timeout (aps.rs:519-528, adserver_mock.rs:340-348, prebid.rs:1141-1145). Justification comments are reasonable; widening ensure_integration_backend with a first_byte_timeout: Duration parameter would let all four sites share one helper.

📝 note

• Testlight stale-Content-Length test is unit-only (testlight.rs:388-412). Extending handle_uses_platform_http_client_with_http_request to inject a stale Content-Length on the stubbed upstream would close the integration-level gap.

Summary

Re-review of the EdgeZero PR13 integration/provider HTTP type migration after the May 12 fix commits and the May 12 merge that imported sourcepoint into PR13.

The prior-review blockers from Apr 14 and Apr 20 (into_bytes()-on-Stream panics, unbounded inbound POST forwarding, lockr Origin expect, GTM 413/502 classification, unbounded testlight POST) are all resolved cleanly — collect_body_bounded / collect_response_bounded are used uniformly across prebid / aps / adserver_mock / testlight / lockr / permutive / datadome / gtm, with thoughtful RTB (2 MiB) vs SDK (16 MiB) sizing. New tests on the auction endpoint, GTM payload limits, and testlight stale Content-Length directly close prior findings.

Two new blockers were introduced by the May 12 sourcepoint merge, plus one outstanding CI/process question.

Blocking

🔧 wrench

• Sourcepoint silently drops streaming JS response bodies — see inline at crates/trusted-server-core/src/integrations/sourcepoint.rs:802. Behavioral regression vs. main. Fix: use collect_response_bounded.

• cargo fmt --all -- --check fails on 8 hunks in sourcepoint.rs — lines 315, 417, 680, 703, 801, 1431, 1472, 1552. Same blocker as the Apr 20 review, reintroduced when sourcepoint was added to PR13 in the May 12 merge. The May 12 "Apply cargo fmt formatting" commit ran before the sourcepoint merge. Fix: cargo fmt --all.

❓ question

• CI gates still aren't enforced for this PR. .github/workflows/format.yml and test.yml still trigger on pull_request: branches: [main]. PR 626's base is feature/edgezero-pr12-handler-layer-types, so only "Integration Tests" ran on commit 28205a00 — fmt, clippy, and cargo test --workspace did not. Combined with the fmt failure above, the PR description's CI checkboxes can't be relied on. Same question as the Apr 20 review; no workflow change has been made. Is the plan to (a) widen workflow triggers to include the stacked feature/edgezero-* base branches, (b) require stacked branches to retarget main before merge, or (c) fold format/clippy/test into the integration-tests workflow that does run?

  Verified locally on commit 28205a00: cargo clippy --workspace --all-targets --all-features -- -D warnings passes; cargo test --workspace passes (954 unit tests in core); cargo fmt --all -- --check fails.

Non-blocking

🤔 thinking

• Orchestrator drops failing provider identity on select() error — crates/trusted-server-core/src/auction/orchestrator.rs:436-440. When select_result.ready is Err, no AuctionResponse::error(provider, …) is pushed. The backend_to_provider map still holds entries for the in-flight providers, but the failed PlatformPendingRequest is consumed and there's no way to look the provider up. The failing backend vanishes from the result set silently — only a log::warn! with the error message, no provider name. The responses array under-counts launched providers. Same concern as the Apr 14 review.

  Suggested fix: preserve the launch-time mapping in a separate Vec<(backend_name, provider_name)> indexed by the position of each PlatformPendingRequest, so the Err arm can produce a proper AuctionResponse::error(provider_name, …).

• Deadline drop and effective_timeout == 0 paths are untested — crates/trusted-server-core/src/auction/orchestrator.rs:446-453 (deadline check) and :294 (zero-timeout skip). The in-tree TODO at :740-754 explicitly acknowledges this. Both paths are load-bearing for the async migration's correctness.

  Suggested fix: introduce a thin abstraction around PlatformHttpClient::select() that takes pre-baked futures, plus a unit test that asserts the loop returns within deadline + ε when one provider is artificially slow.

• GTM accumulated_text buffer can persist stale state across requests — crates/trusted-server-core/src/integrations/google_tag_manager.rs:194,629,641. The Arc<GoogleTagManagerIntegration> is shared across requests, and the accumulator is drained only when is_last_in_text_node. If an upstream HTML stream errors mid-script (or lol_html aborts the document), residual fragments remain in the buffer and get concatenated into the next request's first matching script — corrupting the rewrite output for an unrelated document. Pre-existing (present in main); flagged here because PR13 is the natural owner of the integration layer. Could be a 📌 follow-up issue if you'd prefer to keep PR scope tight.

♻️ refactor

• Auction providers still construct BackendConfig inline — crates/trusted-server-core/src/integrations/aps.rs:519-529, crates/trusted-server-core/src/integrations/adserver_mock.rs:340-350, crates/trusted-server-core/src/integrations/prebid.rs:1521-1525. Providers diverge from proxies on this single call because they need a per-request timeout, while ensure_integration_backend in integrations/mod.rs hardcodes 15 s. Widening the helper to accept an Option<Duration> for the timeout would let three call sites reuse it. Same refactor opportunity flagged in the prior review; non-blocking.

🌱 seedling

• A streaming-aware rewrite helper would close the sourcepoint class of bugs. Sourcepoint's EdgeBody::Stream(_) regression is symptomatic of every "rewrite body" path needing to convert Stream → Once. If something like rewrite_response_body(body, max, integration, |s| rewrite_fn(s)) lived in integrations/mod.rs, integrations couldn't accidentally skip the Stream variant.

CI Status

• fmt: FAIL locally (CI did not run for this commit — see ❓ question)
• clippy: PASS locally (CI did not run)
• rust tests: PASS locally — 954 unit tests in trusted-server-core (CI did not run)
• Integration Tests workflow: PASS (only workflow triggered on 28205a00)

Summary

Re-review of the EdgeZero PR13 integration/provider HTTP type migration after the May 28 fix commits (f5515440, 2b17d4c9). The two prior blockers — sourcepoint streaming-body drop and the cargo fmt failures — are genuinely fixed, as are the lockr Origin panic, the 413/502 classification, and the testlight stale Content-Length. The first_byte_timeout refactor is correct and preserves proxy behavior.

However, the fix for the previously non-blocking "orchestrator drops failing provider identity" finding introduces a new production correctness bug (detailed inline), and a separate stale Content-Length forwarding issue in datadome needs to be addressed. Verdict: REQUEST_CHANGES.

Blocking

🔧 wrench

• Orchestrator mis-identifies the failed provider on select() error in production — inline at auction/orchestrator.rs:436. The diff-against-remaining-names logic relies on backend_name() surviving in remaining, which the Fastly adapter strips to None (works under the test mock, fails in production: blames an arbitrary provider and can drop that provider's real later response).
• datadome forwards a client-supplied stale Content-Length onto a re-framed body — inline at integrations/datadome.rs:387.

❓ question

• CI gates still don't run for this PR. format.yml / test.yml trigger on pull_request: branches: [main], and this PR's base is the stacked feature/edgezero-pr12-handler-layer-types, so only "Integration Tests" ran in CI — fmt, clippy, and cargo test --workspace did not. I verified all three pass locally on 2b17d4c9 (fmt clean, clippy clean with -D warnings, 982 core unit tests pass), but the PR's CI checkboxes can't be relied on as a gate. Is the plan to (a) widen workflow triggers to the stacked feature/edgezero-* bases, (b) retarget to main before merge, or (c) fold fmt/clippy/test into the integration-tests workflow that does run? (Same question as prior reviews; no workflow change has been made.)

Non-blocking

🤔 thinking

• Deadline-drop and effective_timeout == 0 paths remain untested — auction/orchestrator.rs (TODO at ~761). Load-bearing for the async migration's correctness; the in-tree TODO acknowledges it. Aligning the mock select() to strip backend_name from remaining exactly like the Fastly adapter would also have surfaced the wrench finding above — worth doing alongside an Err-path test with ≥2 in-flight providers.

🌱 seedling

• A streaming-aware rewrite helper would close the sourcepoint class of bugs. A shared rewrite_response_body(body, max, integration, |s| ...) in integrations/mod.rs would structurally prevent integrations from accidentally skipping the Body::Stream variant.

👍 praise

• All four prior blockers are genuinely fixed: sourcepoint Stream-body drop now uses collect_response_bounded + a Content-Length guard (sourcepoint.rs:805-811); cargo fmt passes; testlight drops the stale Content-Length with a dedicated regression test (testlight.rs:121,394); GTM 413-vs-502 classification is correct.
• The first_byte_timeout refactor is correct — None.unwrap_or_else(|| 15s) exactly preserves proxy behavior, all three RTB providers thread the auction timeout, and the predicted vs. ensured backend names agree.
• Bounded body reads are uniform and correctly classified: inbound oversize → 413 propagated via a bare ? (no change_context rewriting it to 502), upstream oversize → 502; the lockr Origin expect() is replaced with a warn-and-skip fallback.
• main.rs drops two compat round-trips per proxy request — a genuine cleanup.

CI Status (verified locally on head 2b17d4c9)

• fmt: PASS
• clippy: PASS (-D warnings)
• rust tests: PASS (982 core unit tests)
• Integration Tests (CI): PASS
• fmt / clippy / cargo test --workspace in CI: did not run (see ❓ question)

Summary

Migration of the integration/provider layers from Fastly request/response types to platform-agnostic http types (async request_bids, PlatformPendingRequest/PlatformResponse, threaded RuntimeServices). Reviewed against the stacked base feature/edgezero-pr12-handler-layer-types. The change is clean and well-hardened: no Fastly/compat leakage remains in trusted-server-core, response/request body reads are centrally bounded with distinct RTB/SDK/request caps, stale Content-Length is dropped on every body-rewrite path, and select() errors are now correctly attributed to a provider via the new failed_backend_name. No blocking issues found.

Non-blocking

🤔 thinking

• Timed-out providers vanish from the response: deadline-dropped requests produce no AuctionResponse (unlike launch failures) — consider emitting a timeout error per dropped backend (orchestrator.rs:513).
• Inconsistent error metadata: transport failures push a bare error while parse failures attach error_type/message (orchestrator.rs:493 vs 471).

📌 out of scope

• backend_to_provider keyed only by backend name: providers sharing a backend collide; one response is dropped/misattributed. Pre-existing in base; worth a follow-up guard (orchestrator.rs:379).
• Timeout-enforcement paths untested: acknowledged TODO — deadline drop, mediator skip, and effective_timeout == 0 skip have no coverage; StubHttpClient could now exercise some (orchestrator.rs:950).

⛏ nitpick

• Repeated total_time_ms: 0 placeholder across three OrchestrationResult constructions (orchestrator.rs:172/244/263).

CI Status

• browser integration tests: PASS
• integration tests: PASS
• prepare integration artifacts: PASS